Skip to content

feat(rollup-plugin): expose version#5194

Merged
wjhsf merged 3 commits intosalesforce:masterfrom
cardoso:rollup-plugin-version
Feb 6, 2025
Merged

feat(rollup-plugin): expose version#5194
wjhsf merged 3 commits intosalesforce:masterfrom
cardoso:rollup-plugin-version

Conversation

@cardoso
Copy link
Contributor

@cardoso cardoso commented Feb 5, 2025

Details

This PR exposes the rollup plugin version to facilitate interoperability with other plugins, which is required for supporting scenarios like #4628 in the long run.

See: rollup/rollup#4771

Does this pull request introduce a breaking change?

  • 😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

  • 🤞 No, it does not introduce an observable change.

GUS work item

@cardoso cardoso requested a review from a team as a code owner February 5, 2025 17:35
Copy link
Contributor

@wjhsf wjhsf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to do this? Why can't downstream tooling just read @lwc/rollup-plugin's package.json?

@cardoso
Copy link
Contributor Author

cardoso commented Feb 5, 2025

@wjhsf that's impractical due to differences between package managers, node versions, module systems.

See a similar pull request here: rollup/plugins#1050

Also related 😭 : https://github.com/salesforce/lwc/pull/4897/files/c8e5fbb53df8d311bb4e232b1d0be7b8745904c3#r1854880767

@cardoso
Copy link
Contributor Author

cardoso commented Feb 5, 2025

TL;DR: @lwc/rollup-plugin might be coming in as a peer dependency and there's no specification around finding its package.json, unless the package.json exposes itself in the exports field, and that still requires (ha!) assuming a bunch of stuff.

@divmain
Copy link
Contributor

divmain commented Feb 6, 2025

The impetus for this seems reasonable, and I don't see much of a downside. So 👍 to the PR.

@cardoso
Copy link
Contributor Author

cardoso commented Feb 6, 2025

Thanks @divmain ! My immediate goal here is future-proofing the workarounds in vite-plugin-lwc for consumers other than myself.

I'm also hoping that eventually #4628 is incrementally fixed. Then other plugins can start to work alongside instead of resorting to wrapping/patching a specific version.

In my interpretation:

  • there are no downsides.
  • there are probably no immediate upsides.
  • there are many potential future upsides.

@cardoso
Copy link
Contributor Author

cardoso commented Feb 6, 2025

Karma tests tried to run with missing envs 🫠

@wjhsf
Copy link
Contributor

wjhsf commented Feb 6, 2025

/nucleus test

@wjhsf wjhsf merged commit 306a8f0 into salesforce:master Feb 6, 2025
14 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants